Offline mode implementation #1118

cardenaso11 · 2023-10-17T03:05:35Z

TODO:

Remove commit 4350c87 (@ch1bo)

CHANGELOG updated or not needed
Documentation updated or not needed
Haddocks updated or not needed
No new TODOs introduced or explained herafter

locallycompact · 2023-10-17T09:48:31Z

Do you expect a review now or do you want to wait?

hydra-node/exe/hydra-node/Main.hs

cardenaso11 · 2023-10-19T03:26:15Z

hydra-node/exe/hydra-node/Main.hs

+  withCardanoLedgerOffline OfflineConfig{ledgerGenesisFile} protocolParams action = do
+    --TODO(Elaine): double check previous messy branch for any other places where we query node
+    genesisParameters <- readJsonFileThrow (parseJSON @(Ledger.ShelleyGenesis StandardCrypto)) ledgerGenesisFile
+    globals <- newGlobals $ fromShelleyGenesis genesisParameters


We're reading in genesis parameters here, and while I don't think they're a huge obstacle to obtain, I think they're a necessity because of the type of applyTxes. Is this a correct assessment? Is there any way to sidestep this requirement currently?

Not really. You can have a look here: https://github.com/input-output-hk/hydra/blob/e6f25045cb01e74fce3c63a555fb727060c8a4b1/hydra-node/src/Hydra/Chain/Direct/Fixture.hs#L51
In the tests we are using the data structures to prime a Cardano ledger, you could do something similar in off-line mode

cardenaso11 · 2023-10-19T03:32:50Z

hydra-node/exe/hydra-node/Main.hs

+--TODO(ELAINE): figure out a less strange way to do this
+-- | Taken from Cardano.Api.GenesisParameters, a private module in cardano-api
+
+fromShelleyGenesis :: Shelley.ShelleyGenesis Ledger.StandardCrypto -> GenesisParameters ShelleyEra


If we really do need genesis parameters, is there a less awkward way to get them than parsing a ShelleyGenesis and running a copied fromShelleyGenesis on it? It's a private function so I'm not sure if this is exposing implementation details

See my answer above

cardenaso11 · 2023-10-19T10:54:31Z

hydra-node/src/Hydra/Chain/Direct.hs

@@ -164,6 +178,68 @@ mkTinyWallet tracer config = do
    hoistEpochInfo (first show . runExcept) $
      Consensus.interpreterToEpochInfo interpreter

+withOfflineChain ::
+  Tracer IO DirectChainLog -> -- TODO(ELAINE): change type maybe ?


Is it okay to reuse DirectChainLog here, or should we have a different type to indicate this is offline mode?

You would need to add specific constructors for the specific loggging you would do in off-line mode?

cardenaso11 · 2023-10-19T10:55:45Z

Do you expect a review now or do you want to wait?

A review now I think would be appropriate. Thank you

cardenaso11 · 2023-11-04T10:02:36Z

This needs new tests to ensure the offline mode functionality but I think should otherwise be ready for review

ch1bo

Very nice work so far. As you already mentioned, tests are definitely needed for this and I left a few more remarks on what could be improved.

When you rebase onto the latest master, you can ensure your changes are formatted correctly (which is checked by the CI now) by simply calling treefmt from the nix shell.

ch1bo · 2023-11-07T15:34:41Z

hydra-node/src/Hydra/Options.hs

@@ -161,6 +230,7 @@ data RunOptions = RunOptions
  , persistenceDir :: FilePath
  , chainConfig :: ChainConfig
  , ledgerConfig :: LedgerConfig
+  , offlineConfig :: Maybe OfflineConfig


I see that Hydra.Chain.Offline should be a drop-in replacement for Hydra.Chain.Direct. That makes a lot of sense to me.

Concretely, the hydra-node exe seems to switch a lot on the actual Options provided and does things differently.

I think it would be worthwhile to model this already her using something like

data ChainConfig = DirectChainConfig { ... } | OfflineChainConfig { ... }

along with appropriate Parser ChainConfig which switches on the mutually exclusive command line options which determine the mode of operation.

My hunch is, that modelling it that way, will allow the main to separate more cleanly between the two modes.

ch1bo · 2023-11-07T15:38:45Z

hydra-node/golden/RunOptions.faulty.json

Please avoid committing test output files. Both, faulty.json and reencoded.json, are outputs of the golden test suite which (rightfully) need updating when command line options change.

ch1bo · 2023-11-07T15:51:05Z

hydra-node/exe/hydra-node/Main.hs

+                loadGlobalsFromGenesis ledgerGenesisFile
+
+        withCardanoLedger pparams globals $ \ledger -> do
+          persistence <- createStateChangePersistence (persistenceDir <> "/state") (offlineOptionsNormalizedUtxoWriteBackFilePath =<< leftToMaybe onlineOrOfflineConfig)


What's the rationale of this piggy-backing onto the standard persistence of StateChanged events?

It seems like you are interested in storing the latest utxo at some specific location (in addition to the state file), but I would like to know the whole user story for this.

This mechanism seems quite orthogonal to the offline mode chain layer and I'd prefer having this in a dedicated PR (in which I would ask for the purpose of this the same way).

Depending in how the downstream usage of this file is, a more modular alternative could be a small utility subscribing to the API (websocket) and writing this file on every SnapshotConfirmed message.

Another alternative would be to make this "writeBack" mechanism be attached as a consumer of those StateChanged events here. This would support also an idea we had where the Effects of the business logic (e.g. sending output to clients on the API) be mere interpretations of the event stream.

I think the main concern is that in offline mode, we don't have any "Commit" events, so it's decidedly useless: the head opens with an empty UTXO, and so we can't spend any transactions;

So, we added the ability to specify the initial UTXO on the command line, but in a call we had with the team, there was concerns about where to get this initial UTXO file in the first place; as a convenience, we agreed to add something that saved the UTXO state out when the node shut down gracefully, so we could reuse that on the next run.

If it feels a bit messy / arbitrary, we can strip that back out and say that you need to construct the UTXO by hand, and/or fold this into our other ADR (a PR for which is coming soon!)

I see. But why would you open it empty? Also wouldn't the normal persistence of internal state have the hydra-node start up in the Open state with just the same UTxO as on the last run?

In my mind the offline mode could work like the following:

Always simulates Init and Commit observations based on some configured initial utxo file

When no previous state is loaded, the head logic would interpret that as opening of the head

Allow normal off-chain transaction processing, evolving the Open off-chain head state (which is automatically persisted)

On later starts, the head would already be in an Open state as loaded from persistence and any (simulated) Init and Commit observations from the offline chain would be ignored

The initial UTxO file would be given by your outer gummiworm protocol I suppose.

ch1bo · 2023-11-07T15:56:21Z

hydra-node/src/Hydra/Chain/Offline/Handlers.hs

+                      OnContestTx{snapshotNumber = number $ getSnapshot confirmedSnapshot}}
+                FanoutTx{} ->
+                  callback $ Observation { newChainState = cst, observedTx =
+                      OnFanoutTx{}}


Can an offline chain even open/close a head? I would have expected the offline chain to be in an Open state right away (constantly yielded Observation when starting it) and also ignore all commands to CloseTx it.

ch1bo · 2023-11-07T15:59:08Z

hydra-node/src/Hydra/Chain/Offline.hs

+withOfflineChain tracer OfflineConfig{ledgerGenesisFile} [email protected]{systemStart} ownHeadId chainStateHistory callback action = do
+
+
+  localChainState <- newLocalChainState chainStateHistory


As you point out in a NOTE further up, we do not need to keep a chain state in the offline chain layer, as it is not needed to construct transactions (in fact there are no L1 transactions). So we can keep it initialChainState = ChainStateAt{chainState = Idle, recordedAt = Nothing}

If you apply my suggestions, this becomes unused.

ch1bo · 2023-11-07T16:00:11Z

hydra-node/exe/hydra-node/Main.hs

+            Nothing -> pure ()
+            Just (OfflineConfig {initialUTxOFile})-> do
+              initialUTxO :: UTxOType Tx <- readJsonFileThrow (parseJSON @(UTxOType Tx)) initialUTxOFile
+              initializeStateIfOffline chainStateHistory initialUTxO headId party contestationPeriod (putEvent . OnChainEvent)


I think this (yielding of constant observations to simulate a head opening) can be moved to withOfflineChain directly, which would simplify the main function (less casing).

ghost · 2023-11-29T10:05:28Z

@cardenaso11 How are things going? Is this PR ready for review, or perhaps another pairing session could be helpful?

cardenaso11 · 2023-11-29T20:08:40Z

@cardenaso11 How are things going? Is this PR ready for review, or perhaps another pairing session could be helpful?

I think it is ready for review

ghost · 2023-11-30T10:45:07Z

@cardenaso11 Could you rebase the branch on latest master and resolve conflicts?

ghost · 2023-12-06T13:46:42Z

I noticed a few tests failing, would you need help on this @cardenaso11 ?

cardenaso11 · 2023-12-07T00:49:13Z

@abailly-iohk should be fixed now, had some nix changes for dealing with arm64 macos I didn't realize I checked in that got mangled during rebase somehow

…nChainId" This reverts commit 84adba8.

… merge as is

…, do not merge as is" This reverts commit 6599c0a.

…o have conflicts minimized, warnings and debug code removed. this'll get rebased

Co-authored-by: card <[email protected]>

ch1bo

All must issues are resolved now and I can approve merging this.

I will follow-up with a PR that should significantly simplify the Hydra.Options handling and downstream code duplication in Hydra.Node.Run and HydraNode modules.

ghost

Some very minor comments

ghost · 2023-12-19T17:03:15Z

hydra-cluster/src/HydraNode.hs

+ where
+  logFilePath = workDir </> "logs" </> "hydra-node-" <> show hydraNodeId <.> "log"
+
+withOfflineHydraNode' ::


I don't think we need this separation as withOfflineHydraNode' is used in only one place. And even the other 2 functions are clunky and should be refactored.

I'll cover this in a refactor PR.

cardenaso11 commented Oct 19, 2023

View reviewed changes

cardenaso11 force-pushed the offline-mode-nicer branch from 406c2e9 to 0c3a59f Compare October 20, 2023 11:14

ffakenz assigned cardenaso11 Oct 24, 2023

cardenaso11 force-pushed the offline-mode-nicer branch from c080e0d to 4064c81 Compare November 3, 2023 06:35

cardenaso11 marked this pull request as ready for review November 4, 2023 09:59

cardenaso11 changed the title ~~WIP: initial untested gummiworm-redacted mostly-cleaned-up offline mode implementation~~ Offline mode implementation Nov 4, 2023

cardenaso11 force-pushed the offline-mode-nicer branch from 6f94fe6 to d042f6d Compare November 4, 2023 10:09

ch1bo self-assigned this Nov 6, 2023

ch1bo reviewed Nov 7, 2023

View reviewed changes

cardenaso11 force-pushed the offline-mode-nicer branch 3 times, most recently from 3ba59ac to ea06cfd Compare November 8, 2023 10:11

ch1bo mentioned this pull request Nov 8, 2023

Streaming adr #1157

Merged

4 tasks

cardenaso11 force-pushed the offline-mode-nicer branch from 147905d to 8593410 Compare November 16, 2023 15:49

ch1bo assigned ghost and unassigned ch1bo Nov 17, 2023

cardenaso11 force-pushed the offline-mode-nicer branch from f9ae686 to e995f48 Compare November 29, 2023 18:58

ghost requested review from a user and ch1bo November 30, 2023 10:44

cardenaso11 force-pushed the offline-mode-nicer branch from e995f48 to 9063377 Compare December 6, 2023 07:03

ch1bo unassigned cardenaso11 and ghost Dec 13, 2023

cardenaso11 and others added 16 commits December 18, 2023 15:04

Revert "demonstrate offline mode running with arbitrary participant O…

1228373

…nChainId" This reverts commit 84adba8.

wip: temporary commit for demoing offline mode. Good for demo, do not…

f81761a

… merge as is

Revert "wip: temporary commit for demoing offline mode. Good for demo…

c730f27

…, do not merge as is" This reverts commit 6599c0a.

pr feedback

2c2f4f9

revert back to old port: see cardano-scaling#1218 for macOS port bug

65b2357

more pr feedback

510d4ef

more feedback, undo error->die feedback due to introduced test failure

7f283ae

wip: separate top-level command and run(offline)options type. needs t…

e2438b3

…o have conflicts minimized, warnings and debug code removed. this'll get rebased

add to changelog

b4d21c4

Co-authored-by: card <[email protected]>

randomize utxo in offline test

a892dd3

Co-authored-by: card <[email protected]>

remove dead code

266fc0a

Co-authored-by: card <[email protected]>

remove TODOs

b860d64

Co-authored-by: card <[email protected]>

Hydra.Options: import toArgs from Online

f0a8489

Co-authored-by: card <[email protected]>

resolve warnings

6880688

Co-authored-by: card <[email protected]>

update log schema

3500d51

fix test failures

a10c492

ch1bo force-pushed the offline-mode-nicer branch from 77199b8 to a10c492 Compare December 18, 2023 14:04

ch1bo and others added 5 commits December 18, 2023 15:08

Fix formatting

e373bce

address some feedback

bd68a8e

deduplicate fromShelleyGenesis

0306ce8

move back loadState

6a54f77

organize imports

bea0b76

ch1bo approved these changes Dec 19, 2023

View reviewed changes

ghost approved these changes Dec 19, 2023

View reviewed changes

ch1bo merged commit c92753a into cardano-scaling:master Dec 19, 2023
18 checks passed

Quantumplation deleted the offline-mode-nicer branch December 19, 2023 17:07

ch1bo mentioned this pull request Dec 20, 2023

Refactor offline-mode #1222

Merged

4 tasks

ch1bo added this to the 0.15.0 milestone Dec 22, 2023

ch1bo linked an issue Jan 16, 2024 that may be closed by this pull request

Offline mode #1254

Closed

ghost mentioned this pull request Jan 17, 2024

Offline mode #1254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offline mode implementation #1118

Offline mode implementation #1118

cardenaso11 commented Oct 17, 2023 •

edited by ch1bo

Loading

locallycompact commented Oct 17, 2023

cardenaso11 Oct 19, 2023

ghost Oct 19, 2023

cardenaso11 Oct 19, 2023

ghost Oct 19, 2023

cardenaso11 Oct 19, 2023

ghost Oct 19, 2023

cardenaso11 commented Oct 19, 2023

cardenaso11 commented Nov 4, 2023

ch1bo left a comment

ch1bo Nov 7, 2023

ch1bo Nov 7, 2023

ch1bo Nov 7, 2023

Quantumplation Nov 7, 2023

ch1bo Nov 13, 2023

ch1bo Nov 7, 2023

ch1bo Nov 7, 2023

ch1bo Dec 15, 2023

ch1bo Nov 7, 2023

ghost commented Nov 29, 2023

cardenaso11 commented Nov 29, 2023

ghost commented Nov 30, 2023

ghost commented Dec 6, 2023

cardenaso11 commented Dec 7, 2023

ch1bo left a comment

ghost left a comment

ghost Dec 19, 2023

ch1bo Dec 19, 2023

		withOfflineChain tracer OfflineConfig{ledgerGenesisFile} [email protected]{systemStart} ownHeadId chainStateHistory callback action = do


		localChainState <- newLocalChainState chainStateHistory

Offline mode implementation #1118

Offline mode implementation #1118

Conversation

cardenaso11 commented Oct 17, 2023 • edited by ch1bo Loading

locallycompact commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardenaso11 commented Oct 19, 2023

cardenaso11 commented Nov 4, 2023

ch1bo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 29, 2023

cardenaso11 commented Nov 29, 2023

ghost commented Nov 30, 2023

ghost commented Dec 6, 2023

cardenaso11 commented Dec 7, 2023

ch1bo left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardenaso11 commented Oct 17, 2023 •

edited by ch1bo

Loading